Skip to content

fix significant_drop_tightening: don't suggest when missing the necessary spans #15598

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

fix significant_drop_tightening: don't suggest when missing the necessary spans #15598

wants to merge 2 commits into from

Conversation

@ada4a
Copy link
Contributor

@ada4a ada4a commented Aug 31, 2025 •
edited
Loading

This improves the situation in #15574 a bit: instead of making a wrong suggestion, we give a correct, albeit imprecise, help message -- notice how its span doesn't point exactly at the "heavy" type, but just the statement it's located in.

I initially wanted to implement this by replacing DUMMY_SP with None, but there are some other places in the code where the former is used which are hard to get rid of (for example dummy_stmt_expr, which is fishy in and of itself), so I resorted to checking is_dummy before emitting the suggestion.

changelog: [significant_drop_tightening]: don't suggest when missing the necessary spans

@rustbot
Copy link
Collaborator

rustbot commented Aug 31, 2025

r? @Alexendoo

rustbot has assigned @Alexendoo.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Aug 31, 2025
@ada4a
Copy link
Contributor Author

ada4a commented Aug 31, 2025

r? clippy

@rustbot rustbot assigned Jarcho and unassigned Alexendoo Aug 31, 2025
@ada4a ada4a force-pushed the significant_drop_tightening branch 2 times, most recently from 03a2c76 to e4797f5 Compare August 31, 2025 17:17
@github-actions
Copy link

github-actions bot commented Aug 31, 2025 •
edited
Loading

Lintcheck changes for 646705a

Lint Added Removed Changed
clippy::significant_drop_tightening 0 0 54

This comment will be updated if you push new changes

@ada4a
Copy link
Contributor Author

ada4a commented Aug 31, 2025

Uhh, that doesn't look right..

@ada4a ada4a marked this pull request as draft September 1, 2025 20:31
@rustbot rustbot removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Sep 1, 2025
@ada4a ada4a force-pushed the significant_drop_tightening branch 2 times, most recently from 1809efa to e6f19ec Compare September 2, 2025 10:28
@ada4a ada4a changed the title fix significant_drop_tightening: don't suggest when not having the necessary spans fix significant_drop_tightening: don't suggest when missing the necessary spans Sep 2, 2025
@ada4a ada4a force-pushed the significant_drop_tightening branch from e6f19ec to 3e414c4 Compare September 2, 2025 10:46
@ada4a
Copy link
Contributor Author

ada4a commented Sep 2, 2025

Much better

@ada4a ada4a marked this pull request as ready for review September 2, 2025 10:58
@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Sep 2, 2025
ada4a
ada4a commented Sep 2, 2025
View reviewed changes
clippy_lints/src/significant_drop_tightening.rs Outdated
);
},
} else {
diag.help("merge the temporary construction with its single usage");
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure whether I should turn this into span_help, given that the span this points at is imprecise -- it shows the span in which the lock is used, and not the exact place that the temporary construction should be merged into

@ada4a ada4a force-pushed the significant_drop_tightening branch 2 times, most recently from 7db57b0 to 92b0021 Compare September 2, 2025 13:06
@ada4a ada4a mentioned this pull request Sep 2, 2025
Open
Jarcho
Jarcho requested changes Sep 25, 2025
View reviewed changes
clippy_lints/src/significant_drop_tightening.rs

fn has_drop(expr: &hir::Expr<'_>, first_bind_ident: Option<Ident>, lcx: &LateContext<'_>) -> bool {
if let hir::ExprKind::Call(fun, [first_arg]) = expr.kind
if let Some(first_bind_ident) = first_bind_ident
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since you're changing this the function shouldn't even take an Option here.

Copy link
Contributor Author

@ada4a ada4a Sep 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's appropriate in this case, because apa.first_bind_ident is an Option<Ident> as well, no?

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Sep 25, 2025
@ada4a ada4a force-pushed the significant_drop_tightening branch from 92b0021 to eb438b7 Compare September 25, 2025 18:23
@rustbot

This comment has been minimized.

Sign in to view
@ada4a
Copy link
Contributor Author

ada4a commented Sep 29, 2025

@rustbot ready

@rustbot rustbot removed the S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) label Sep 29, 2025
@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Sep 29, 2025
@rustbot

This comment has been minimized.

Sign in to view
@ada4a ada4a force-pushed the significant_drop_tightening branch from eb438b7 to ee374fe Compare October 11, 2025 08:27
@rustbot

This comment has been minimized.

Sign in to view
@rustbot

This comment has been minimized.

Sign in to view
Jarcho
Jarcho approved these changes Oct 16, 2025
View reviewed changes
Copy link
Contributor

@Jarcho Jarcho left a comment •
edited by rustbot
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you. Just needs to be rebased.

View changes since this review

@ada4a ada4a force-pushed the significant_drop_tightening branch from ee374fe to 646705a Compare October 16, 2025 19:50
@rustbot
Copy link
Collaborator

rustbot commented Oct 16, 2025

This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Jarcho Jarcho Jarcho approved these changes

Assignees

@Jarcho Jarcho

Labels

S-waiting-on-review Status: Awaiting review from the assignee but also interested parties

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@ada4a @rustbot @Jarcho @Alexendoo